pr: uniformly scan for form feed and newline chars #10303
Merged
+229
−261
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merging this PR will degrade performance by 3.26%
Performance Changes
Comparing 
Footnotes
|
|
GNU testsuite comparison: |
1 similar comment
|
GNU testsuite comparison: |
Remove unit test for `pr` that was enforcing incorrect behavior. These tests were ostensibly designed to match corresponding ones in the GNU test suite, but they don't match the current behavior of GNU `pr`, so it is not valuable to keep them.
Fix the way form feed characters are interpreted by changing the way lines and pages are found. Before this commit, a file comprising two form feed characters (`/f/f`) would result in too few trailing newlines at the end of the second page. After this change, each page is produced with the correct number of lines. This commit changes the way files are read, replacing complex iterators with a loop-based approach, iteratively scanning for newline or form feed characters. The `memchr` library is used to efficiently scan for these two characters. One downside of this implementation is that it currently reads the entire input file into memory; this can be improved in subsequent merge requests.
jfinkels
changed the title
|
GNU testsuite comparison: |
This was referenced Jan 18, 2026
sylvestre
reviewed
Jan 18, 2026
Comment on lines
+768
to
+776
| // TODO Read incrementally. | ||
| let buf = if path == "-" { | ||
| let mut f = stdin(); | ||
| let mut buf = vec![]; | ||
| f.read_to_end(&mut buf)?; | ||
| buf | ||
| } else { | ||
| std::fs::read(path)? | ||
| }; |
Contributor
There was a problem hiding this comment.
duplicated code with get_file_line_groups
please fix it in a different pr
sylvestre
reviewed
Jan 18, 2026
| ) -> Result<Self, FromUtf8Error> { | ||
| // TODO Don't read bytes to String just to directly write them | ||
| // out again anyway. | ||
| let line_content = String::from_utf8(buf.to_vec())?; |
Contributor
There was a problem hiding this comment.
it doesn't work with buff directly ?
Collaborator
Author
There was a problem hiding this comment.
I was trying to accommodate some existing code in this file that expects a String. It should be okay to just use the bytes, I will fix that in a new merge request. Thanks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix the way form feed characters are interpreted by changing the way
lines and pages are found. Before this commit, a file comprising two
form feed characters (
/f/f) would result in too few trailing newlinesat the end of the second page. After this change, each page is produced
with the correct number of lines.
This merge request changes the way files are read, replacing complex iterators
with a loop-based approach, iteratively scanning for newline or form
feed characters. The
memchrlibrary is used to efficiently scan forthese two characters. One downside of this implementation is that it
currently reads the entire input file into memory; this can be improved
in subsequent merge requests. The behavior of uutils
pris still quite different from GNUpr, so hopefully this is acceptable as a temporary debt to make many more straightforward improvements.This merge request also removes a unit test that was enforcing incorrect behavior. These
tests were ostensibly designed to match corresponding ones in the GNU
test suite, but most of them don't match the current behavior of GNU
pr, so itdoes not seem valuable to keep them. As we improve uutils
pr, we can add more targeted and minimal test cases to replace them.